Header iterator memory optimization in HeaderGroup - #694
Conversation
| this.lastIndex = -1; | ||
| } | ||
|
|
||
| BasicListHeaderIterator(final List<? extends Header> headers, final int currentIndex, final String name) { |
There was a problem hiding this comment.
@ok2c
The new BasicListHeaderIterator constructor does not initialize lastIndex, so it defaults to 0. Calling remove() before next() then removes the first header instead of throwing IllegalStateException. I confirmed it with a regression test. Initializing lastIndex to -1 fixes it.
Otherwise, the change looks good to me.
Test that prove.
@Test
void testIteratorByNameRemoveBeforeNext() {
final HeaderGroup headerGroup = new HeaderGroup();
final Header headerA = new BasicHeader("a", "a-one");
final Header headerB = new BasicHeader("b", "b-one");
headerGroup.setHeaders(headerA, headerB);
final Iterator<Header> iterator = headerGroup.headerIterator("b");
Assertions.assertThrows(IllegalStateException.class, iterator::remove);
Assertions.assertArrayEquals(
new Header[] { headerA, headerB },
headerGroup.getHeaders());
}
There was a problem hiding this comment.
@arturobernalg Good catch! Thank you! Please do another pass. I have also made a few small improvements and optimizations and have added test coverage. Those tests can also serve as an example of how to parse messages efficiently
…#parseElementListStrict to disallow malformed / unparsed elements
26d8ba4 to
d16909e
Compare
There was a problem hiding this comment.
@ok2c The previous implementation inherited argument validation from parseElementList(). With the direct parsing loop those checks are lost.
Could we preserve the existing contract here?
Args.notNull(src, "Source");
Args.notNull(cursor, "Cursor");
Args.notNull(consumer, "Consumer");
Otherwise look good
@arturobernalg I can do that, but would that be really useful? This is a package private constructor not exposed through public APIs and not callable outside of the same package. What good would those checks do? |
@ok2c Fair enough |
arturobernalg
left a comment
arturobernalg
left a comment
There was a problem hiding this comment.
The previous implementation inherited argument validation from parseElementList(). With the direct parsing loop those checks are lost.
Could we preserve the existing contract here?
Args.notNull(src, "Source");
Args.notNull(cursor, "Cursor");
Args.notNull(consumer, "Consumer");
Otherwise look good
2 participants
@arturobernalg Please double-check / review